Table scheme import - #2888
Conversation
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
b73b4af to
2c9d442
Compare
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
blizzz
left a comment
blizzz
left a comment
There was a problem hiding this comment.
Early comments, some can be left for later.
UI wise it looks really nice already!
-
I would not display the UUID as title in the check box. It looks technical and table managers will have not deeper knowledge about it. Would completely keeps this identifier internal and instead use the New Title.
-
In my quick test I also see that Views would be dropped instead of being kept
-
Maybe need to double check the sort order as well when nothing is defined
This is very promising already! :)
| */ | ||
| #[NoAdminRequired] | ||
| #[RequirePermission(Application::PERMISSION_MANAGE, null, 'context', 'contextId')] | ||
| public function importScheme(int $contextId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $tables): DataResponse { |
There was a problem hiding this comment.
Node sure I understand correctly the role of $nodes vs $tables. Maybe have an idea with the TODO against the nodes.
There was a problem hiding this comment.
$nodes is array ids of tables and views to be shown. The $tables is array of related source tables.
| use OCA\Tables\Errors\NotFoundError; | ||
| use OCA\Tables\Errors\PermissionError; | ||
|
|
||
| class StructureService { |
There was a problem hiding this comment.
I know it comes from me, just to note down one thought that circles my mind is whether to add Stateful to the class name for safer usage 🤔 Again, nothing that cannot be improved upon later on.
There was a problem hiding this comment.
I think it can be improved later.
| $columns = $this->columnService->findAllByTable($id, null, $table); | ||
| $this->enhanceTable($table, $userId); | ||
| return new TableScheme($table->getTitle(), $table->getEmoji(), $columns, $table->getViews() ?: [], $table->getDescription() ?: '', $this->appManager->getAppVersion('tables'), $table->getColumnOrderSettingsArray(), $table->getSortArray()); | ||
| $this->enhanceTable($table, $userId ?? $this->userId); |
There was a problem hiding this comment.
i suppose there is a reason for this line change? ^
There was a problem hiding this comment.
It would cause error when $userId is null
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Does it mean that we should remove the
Could you please share more details? |
No, that should be there! In my test, the scheme file I uploaded did not have any changes with the views. However the dialog showed they would be removed, instead of kept. |
Signed-off-by: Luka Trovic <luka@nextcloud.com>
The application scheme includes the table schemes of related source tables. So it also detects the different views of those tables. Should we skip processing removed views when importing application scheme? |
2 participants
🖼️ Screenshots
🏁 Checklist
/backport to stableX.X🤖 AI (if applicable)